tpu-client-next: add close connection event handling#7667
tpu-client-next: add close connection event handling#7667KirillLykov merged 4 commits intoanza-xyz:masterfrom
Conversation
|
@KirillLykov when you get a chance can you take a look |
KirillLykov
left a comment
KirillLykov
left a comment
There was a problem hiding this comment.
Amazing PR, thanks a lot! I left some comments.
| /// The worker proactively monitors connection health while processing transactions, | ||
| /// detecting connection closures immediately rather than waiting for send failures. |
There was a problem hiding this comment.
| /// The worker proactively monitors connection health while processing transactions, | |
| /// detecting connection closures immediately rather than waiting for send failures. | |
| /// The worker proactively monitors connection health while processing | |
| /// transactions, detecting connection closures immediately rather than waiting | |
| /// for send failures. |
There was a problem hiding this comment.
reformatted using rewrap vs plugin
| } | ||
| } | ||
|
|
||
| // Record the error in statistics |
There was a problem hiding this comment.
| // Record the error in statistics |
I think this comment is unnecessary because it is clear from the name of the called function what is going on.
| }; | ||
| self.send_transactions(connection.clone(), transactions) | ||
| .await; | ||
| let conn_monitor = connection.clone(); |
There was a problem hiding this comment.
| let conn_monitor = connection.clone(); |
I don't think it is needed.
| } | ||
|
|
||
| // Monitor connection health proactively | ||
| close_reason = conn_monitor.closed() => { |
There was a problem hiding this comment.
| close_reason = conn_monitor.closed() => { | |
| close_reason = connection.closed() => { |
|
Also Ci fails with trailing whitespaces, could you run |
leafaar
force-pushed
the
tpu-add-close-connection
branch
from
4e48cf4 to
986a41f
Compare
| .expect("Send first batch"); | ||
|
|
||
| // Wait for connection establishment | ||
| sleep(Duration::from_millis(100)).await; |
There was a problem hiding this comment.
Hey, yeah I had two just for "separated" behavior, in the end putting it together will still work. I'm removing and just leaving one. Thanks :D
| sleep(Duration::from_millis(100)).await; | ||
|
|
||
| // Long delay during which connection may be closed | ||
| sleep(Duration::from_secs(2)).await; |
There was a problem hiding this comment.
this will flake if someone changes timeout server side...
leafaar
force-pushed
the
tpu-add-close-connection
branch
from
986a41f to
badbc68
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #7667 +/- ##
=======================================
Coverage 83.0% 83.0%
=======================================
Files 812 812
Lines 356963 357003 +40
=======================================
+ Hits 296593 296656 +63
+ Misses 60370 60347 -23 🚀 New features to boost your workflow:
|
6 participants
Problem
ConnectionWorker does not subscribe to the close event, only discovering connection
failures when send operations fail. This causes:
See yellowstone-jet implementation for reference: https://github.com/rpcpool/yellowstone-jet/blob/main/src/quic_gateway.rs#L786
Summary of Changes
connection.closed()concurrently with transaction processing in Active statehandle_connection_closed()method to log and categorize close reasonsFixes #7517